Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Controller helpers #56

Merged
merged 6 commits into from
May 14, 2021
Merged

Controller helpers #56

merged 6 commits into from
May 14, 2021

Conversation

squaremo
Copy link
Member

@squaremo squaremo commented Nov 26, 2020

This is an experimental PR to see if some of the bits common to all the GOTK controllers can be factored out into embeddable structs.

Among other things, this will

  • reduce duplication
  • standardise how these common bits are done

It is a bit rough around the edges:

You have to supply an ObjectReference to both the metrics and the events methods. This is because all the constituent bits calculated this for themselves, and had different ways of dealing with errors. This way, the error handling is done once, up front, and the ref only has to be calculated once. But it would be nice to do this behind the scenes, in some way.

You have to pass logs in to the Events helper. This is a bit awkward, as getting logs to funcs always is. It'd be nice if the setup could just do this for you.

runtime/controller/metrics.go Outdated Show resolved Hide resolved
runtime/controller/metrics.go Outdated Show resolved Hide resolved
runtime/controller/metrics.go Outdated Show resolved Hide resolved
@squaremo
Copy link
Member Author

squaremo commented Mar 1, 2021

@hiddeco, @seaneagan: is this useful enough to be worth persevering, wdyt?

@hiddeco
Copy link
Member

hiddeco commented Mar 1, 2021

I think that with our comments addressed it would be really useful to get good Kubernetes events and metadata rich notifications. So, yes!

@hiddeco hiddeco force-pushed the controller-shell branch 2 times, most recently from a21e69c to 2144df2 Compare April 14, 2021 13:46
@hiddeco hiddeco force-pushed the controller-shell branch from 2144df2 to 7fbdbe8 Compare May 1, 2021 21:27
@hiddeco hiddeco force-pushed the controller-shell branch from 7fbdbe8 to eb0ad08 Compare May 12, 2021 11:39
@hiddeco hiddeco requested a review from stefanprodan May 12, 2021 11:39
@hiddeco hiddeco marked this pull request as ready for review May 12, 2021 11:40
@hiddeco hiddeco force-pushed the controller-shell branch 2 times, most recently from 408ae15 to 5eaace5 Compare May 12, 2021 11:52
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

Copy link
Member Author

@squaremo squaremo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hidde, your commit messages are a joy 🥭

squaremo and others added 6 commits May 14, 2021 10:25
This adds a struct which can be embedded in a reconciler, which
contains the metrics recording logic.

Signed-off-by: Michael Bridgen <michael@weave.works>
This adds the capability of sending events to Kubernetes and the
notification controller.

Signed-off-by: Michael Bridgen <michael@weave.works>
This commit adds support for providing a reason to the Event method,
and maps the severity of an event to a K8s core event type. This makes
the emitted events in-line with Kubernetes standards, and is beneficial
to the Flux UI: fluxcd/webui#13

The reason to not fully depend on the K8s event type and drop the
severity is because there have been reports from users wanting to have
access to "debug" events, a type not known to Kubernetes.

Signed-off-by: Hidde Beydals <hello@hidde.co>
To allow recording metrics for conditions other than just the readiness
status.

The `RecordReadinessMetric` method is kept in place for API consumption
convenience.

Signed-off-by: Hidde Beydals <hello@hidde.co>
This commit addresses the two rough edges mentioned by Michael.

> You have to supply an ObjectReference to both the metrics and the
> events methods.

As the helper methods are always called from within a reconciler, and
both events and metrics must only be recorderd for well known
resources, the methods of the helpers should only be called for objects
that do contain data about the kind and/or version.

This means that calling `reference.GetReference` within the helper
methods will generally not result in any errors, nor be a really
expensive calculation.

In cases where the GVK information for some reason is not available
on the object, `GetReference` will still be capable of falling back to
the manager's `runtime.Scheme` that is now injected to the helper.

Resulting in a - in my personal opinion - fair trade-off, and a much
more friendly API from the consumer's perspective.

> You have to pass logs in to the Events helper.

By relying on the context of the reconcile operation we get a logger
(with object metadata preconfigured) for free.

Signed-off-by: Hidde Beydals <hello@hidde.co>
Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco hiddeco force-pushed the controller-shell branch from 5eaace5 to 514528d Compare May 14, 2021 08:26
@hiddeco hiddeco merged commit df1bbeb into main May 14, 2021
@hiddeco hiddeco deleted the controller-shell branch May 14, 2021 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants